Skip to content

fix(kernel): stop premature sync statement close (H4); bump KERNEL_REV to batch 2#830

Merged
vikrantpuppala merged 5 commits into
mainfrom
feat/use-kernel-cuj-batch2-fixes
Jun 5, 2026
Merged

fix(kernel): stop premature sync statement close (H4); bump KERNEL_REV to batch 2#830
vikrantpuppala merged 5 commits into
mainfrom
feat/use-kernel-cuj-batch2-fixes

Conversation

@vikrantpuppala
Copy link
Copy Markdown
Contributor

@vikrantpuppala vikrantpuppala commented Jun 4, 2026

What

Connector half of the kernel batch-2 CUJ-gap fixes (databricks-sql-kernel#123, merged). Pins KERNEL_REV to merged kernel main and adds the one connector-side change.

KERNEL_REV bump

KERNEL_REVf4ee6fec78aabce8c0ea9c1ff47fc11b8191d013 — current kernel main, which includes the batch-2 kernel work (#123: sync-statement auto-close-on-drain, server-CANCELLED → cancelled class, U2M refresh fail-fast, metadata statement close-on-drop, per-binding OAuth client_id) plus #120 (logging bridge) and #125 (version bump). This is a merged, non-orphan SHA reachable from main.

Connector change — don't close the kernel Statement at sync execute-return

execute_command's finally used to stmt.close() immediately after a successful stmt.execute(). For a large CloudFetch result with paginated chunk links (all_fetched=false), the kernel fetches later links lazily (get_result_chunks against the live statement) during consumption — so a premature CloseStatement broke those fetches. (The common all_fetched=true case was unaffected, so this only bit very large paginated-link results.)

The kernel now auto-closes the server statement when its result stream drains (#123), with the executed-handle Drop as the backstop for partial/abandoned reads. So the connector now flips close_stmt = False on a successful execute and closes only on the error path (where no executed handle / result set was produced to reap the statement).

The other batch-2 fixes (cancelled-class → OperationalError, U2M refresh fail-fast, metadata close-on-drop, per-binding OAuth client_id) are entirely kernel-side and need no connector code beyond the KERNEL_REV bump.

Testing

  • Unit: sync execute does-not-close-on-success / does-close-on-failure. 230 connector unit tests pass; black (pinned 22.3.0) clean.
  • E2E (live, dogfood, use_kernel=True): large multi-chunk result (5M rows) drains without premature close + cursor reuse; server cancel surfaces as OperationalError (not ProgrammingError); plus the existing staging fail-loud / diagnostic-info / logging-bridge e2e re-verified against the merged kernel.

Kernel #123 is merged; this is ready to merge.

This pull request and its description were written by Isaac.

…V to batch 2

Connector half of the kernel batch-2 fixes (kernel PR #123). Bumps
KERNEL_REV to pick up the batch-2 kernel surface.

H4 — don't close the kernel Statement at sync execute-return:
execute_command's finally used to `stmt.close()` immediately after
`stmt.execute()` succeeded. For a large CloudFetch result with
paginated chunk links (all_fetched=false), the kernel fetches later
links lazily (get_result_chunks against the LIVE statement) during
consumption, so a premature CloseStatement broke those fetches. The
kernel now auto-closes the server statement when its result stream
drains (ExecutedStatement::next_batch end-of-stream), with the
executed-handle Drop as the backstop for partial/abandoned reads. So
the connector now flips close_stmt=False on a successful execute and
only closes on the error path (no executed handle was produced).

The other batch-2 fixes (cancelled-class -> OperationalError, U2M
refresh fail-fast, metadata statement close-on-drop, per-binding OAuth
client_id) are entirely kernel-side and need no connector code beyond
the KERNEL_REV bump.

Tests: unit (sync execute does-not-close on success / does-close on
failure) + e2e (large multi-chunk result drains without premature
close + cursor reuse; server cancel maps to OperationalError not
ProgrammingError). All e2e verified live against dogfood.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
#123 picked up a follow-up commit fixing the wiremock cancelled-state
assertions (ErrorCode::Cancelled). Bump the placeholder pin so the
connector CI builds the corrected kernel. Still to be re-pinned to the
squash-merge SHA before #830 merges.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Kernel batch-2 (#123) is merged to kernel main (f4ee6fe, current main
HEAD). Re-pin from the orphaned branch HEAD (4f7fbe7) to the merged
SHA — reachable from main, no orphan-SHA risk. Verified against a
wheel built from f4ee6fe: connector unit (102) + kernel e2e (H4
large-result drain + reuse, server-cancel -> OperationalError, staging
fail-loud, diagnostic-info) all pass against the real merged kernel.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
…atch2-fixes

Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Comment thread src/databricks/sql/backend/kernel/client.py Outdated
Address review: 'H4' is internal audit shorthand, meaningless in the
public connector codebase. Reword the affected comment + two test
docstrings to describe the behavior directly (premature sync
CloseStatement broke lazy CloudFetch chunk-link fetches). No code change.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala vikrantpuppala added this pull request to the merge queue Jun 5, 2026
Merged via the queue into main with commit 85f8ba3 Jun 5, 2026
43 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants